Repository navigation
fix(drive-abci)!: leave chain locks signed by quorums Platform does not hold to Core (PV15) - #5454
Conversation
…ot hold to Core (PV15) verify_chain_lock_locally v0 rejects (Some(false)) valid chain locks whose signing height (H - 8) falls where Platform does not hold the active quorums: - a quorum mined after the last committed core height C inside the same DKG interval: v0 only defers to Core once the signing height passes the end of C's interval, but commitments are mined mid-interval; - a signing height between the two stored quorum sets after a core height jump across two mining windows: v0 rejects when both sets fail. Seen in the 4.1.1 -> 5.0.0-rc.2 local upgrade rehearsal: with C = 1782 (llmq_test interval 1776..=1799, mining window 1786..=1794), chain locks 1795 and 1800 were rejected by every other validator, and Platform block 1122 took 4 rounds. The logic is unchanged since at least v4.1.1. Liveness only: the rejection is deterministic, so validators agree. verify_chain_lock_locally v1 is v0 with three decision changes: 1. after a failed check against the current quorums, return None (Core decides) when a commitment of the chain lock quorum type can have been mined between C and the signing height, using Dash Core's DKG mining schedule (llmq/params.h) kept in v1; rotating and unknown types defer for any signing height past C; 2. return None when both stored quorum sets fail; 3. drop the round >= 10 branch, which only logged. v1 differs from v0 only by turning some Some(false) into None; every remaining local rejection is on a quorum set Platform holds exactly. The schedule is owned by v1 rather than read from rust-dashcore, whose table has interval 288 for llmq_test_platform and llmq_devnet_platform (Core: 24) and wrong mining offsets for llmq_devnet_dip0024, and so that a dependency bump cannot change protocol v15. DRIVE_ABCI_METHOD_VERSIONS_V11 (V10 with verify_chain_lock_locally: 1) is used only by PLATFORM_V15; protocol v14 and earlier keep v0. #4648 and #4652 also create DRIVE_ABCI_METHOD_VERSIONS_V11; whichever lands second adds its slot to the existing table. Test would have caught this: the dispatcher test for case A and the verify_chain_lock test accepting it through Core fail with PV15's verify_chain_lock_locally slot at 0 (v0) and pass at 1 (v1): ✖ before fix, ✔ after. The first repro tests, written before the fix, returned Some(false) for both cases under the latest protocol version. Mutating the mining window boundaries or restoring the both-sets rejection fails 2 to 3 tests each. Two mainnet chain locks (2555653, 2555665) pin quorum choice, digest and hash byte order to real Core data. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Waiting for bot review — coderabbitai not yet · thepastaclaw requested changes — dismiss the review or push a fix, 1 thread unresolved — resolve it. Wait for the missing reviews, or a writer can post |
|
✅ Final review complete — no blockers (commit 2cd9d44) · triage: critical |
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Static verification at e373931 confirms that the PV15 dispatch preserves historical production behavior, but the newly expanded Core fallback can accept ChainLocks without authenticating their signatures. The overlapping security reports consolidate into one blocker; two nonblocking version-table suggestions are also substantiated by the repository's documented conventions. No builds or tests were run; the supplied CI snapshot reports successful Rust workspace tests, with PR Hygiene pending.
🔴 1 blocking | 🟡 2 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: platform-versioning); reviewer 9: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 10: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); reviewer 11: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 12: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — verify_chain_lock_locally_v1 changes consensus-relevant ChainLock rejection rules through intricate signing-height, quorum-history, and DKG mining-window logic activated at protocol v15. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 14% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort xhigh); agentphase2-reviewer - Model comparison: every Phase-2 reviewer also ran on
gpt-6-astra; the verifier weighed both sets without knowing which model wrote which
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-drive-abci/src/execution/platform_events/core_chain_lock/verify_chain_lock_locally/v1/mod.rs`:
- [BLOCKING] packages/rs-drive-abci/src/execution/platform_events/core_chain_lock/verify_chain_lock_locally/v1/mod.rs:233-238: Authenticate fallback ChainLocks at the Core verification boundary
Returning `None` here delegates to a synchronization path that does not necessarily authenticate the submitted ChainLock. Production `run_block_proposal_v0` calls `verify_chain_lock(..., true, ...)`, so `verify_chain_lock_through_core_v0` calls only `submit_chain_lock`. Dash Core v23.1.2's `src/rpc/quorums.cpp:1223–1224` returns its best ChainLock height immediately when the submitted height is already covered, before parsing or verifying the signature. Platform converts that height into `CoreSyncStatus::Done`; `verify_chain_lock_v0` then returns both validity fields as `None` and synchronization as `Some(true)`, which proposal validation accepts.
The added fixture demonstrates a newly affected input: at committed Core height 1106, an unrelated-key signature at height 1122 has signing height 1114. v0 rejects it locally, but v1 returns `None`—the new boundary test explicitly asserts this. If Core already has a genuine ChainLock at 1122 or later, the forged certificate passes the outer verifier without either a local or Core signature verdict. A Core node below that height instead attempts verification and rejects it. This does not advance Platform beyond Core's genuine best ChainLock height, but it admits unauthenticated certificates and makes proposal acceptance depend on Core's current ChainLock state. The underlying fallback weakness predates this PR, but these new deferrals expose inputs that were previously rejected.
Add a PV15-gated fallback that explicitly authenticates deferred ChainLocks through `CoreRPCLike::verify_chain_lock`, propagates a negative verdict to proposal validation, and performs submission separately for synchronization. Keep historical generations unchanged. Add an outer-verifier regression for the forged height-1122 case with Core already at or beyond that height; the existing outer-verifier forgery test only covers height 1121, which is still rejected locally.
- [SUGGESTION] packages/rs-drive-abci/src/execution/platform_events/core_chain_lock/verify_chain_lock_locally/v1/mod.rs:268-270: Keep DKG timing parameters in the protocol version tables
These schedules control whether a failed signature check rejects a proposal or delegates it to Core, making them protocol parameters rather than implementation-only details. Freezing the Core values instead of reading dependency-owned parameters is appropriate, but `book/src/versioning/platform-version.md` explicitly places constants that may change in a future protocol version in the version tables, and `feature-versions.md` documents `DriveAbciCoreChainLockMethodVersionsAndConstants` as a suitable owner. Store the frozen schedules there and read them through `platform_version`, leaving historical v0 behavior unchanged. This preserves the dependency-upgrade protection while allowing a future schedule-only change through a new snapshot rather than another copied implementation generation. The current generation gate is replay-safe, so this is nonblocking.
In `packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v11.rs`:
- [SUGGESTION] packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v11.rs:21-26: Construct V11 as an explicit delta from V10
The new table copies every method slot although its only value change from V10 is `core_chain_lock.verify_chain_lock_locally`. The new-table convention in `book/src/versioning/feature-versions.md` explicitly requires struct-update syntax. Initialize `core_chain_lock` with `verify_chain_lock_locally: 1` and `..DRIVE_ABCI_METHOD_VERSIONS_V10.core_chain_lock`, then inherit the remaining fields with `..DRIVE_ABCI_METHOD_VERSIONS_V10`. The copied values currently match, so this is not an activation defect; expressing the one-slot delta structurally makes unrelated compatibility easier to verify and prevents accidental divergence during subsequent edits.
… as on v5.0-dev #5459 selects verify_chain_lock_locally v1 in place at protocol 14 on v5.0-dev, so it ships in 5.0.0-rc.4. Protocol 15 inherits protocol 14's tables (PLATFORM_V15 uses DRIVE_ABCI_METHOD_VERSIONS_V10), so a separate table for protocol 15 is no longer needed: - drop DRIVE_ABCI_METHOD_VERSIONS_V11 and point nothing new at v15.rs, which is again functionally identical to v14; - DRIVE_ABCI_METHOD_VERSIONS_V10 sets verify_chain_lock_locally to 1, and v14.rs lists it as item 96, with the same text as #5459 so the forward merge of v5.0-dev sees identical edits; - the gate test now checks protocol 13 (v0) against 14 and the latest (v1), the v0-against-v1 test runs v0 at protocol 13, and v0's own test is pinned to protocol 13; - the DKG schedule test covers all eleven non-rotating quorum types and llmq_devnet_dip0024 among the rotating ones. Replay-safe for protocol 14 blocks committed under v0: v1 answers differently only where v0 answers Some(false), which refuses the proposal, so every block v0 accepted is accepted the same way by v1. Test would have caught this: as in #5459, the dispatcher gate test and the verify_chain_lock accept-through-Core test fail with protocol 14's slot at 0 and pass at 1 (✖ before, ✔ after). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Waiting for bot review — coderabbitai not yet · thepastaclaw 1 thread unresolved — resolve it. Wait for the missing reviews, or a writer can post |
…rifychainlock (PV14) A chain lock that verify_chain_lock_locally v1 leaves to Core (None) went straight to Core's submitchainlock. Dash Core returns its best chain lock height from submitchainlock without checking the signature when that height is at or above the submitted one (rpc/quorums.cpp, v23.1.8), and Drive turns that answer into "Core is synced". So a Byzantine proposer's forged chain lock at an already chain-locked height was accepted by every validator whose Core had a chain lock at or above it. verify_chain_lock_locally v1 now asks Core's verifychainlock whenever the check against the quorums Platform holds cannot decide, on every such path: - false: Some(false), the proposal is rejected and submitchainlock is not called; - true: None, unchanged: submitchainlock still decides whether Core is synced; - RPC error: Err(CoreRpc), which run_block_proposal turns into InvalidChainLock, as it already does for a submitchainlock error there. The check against the held quorums moves unchanged into verify_chain_lock_with_held_quorums, which the local-logic tests call. A false from Core is logged as a warning naming both causes (an invalid signature, or Core missing the block at H - 8); the proposal error is the existing local-rejection one. Both RPCs run the same chainlock::VerifyChainLock. For a genuine chain lock above Core's best they give the same verdict; at or below it, Core's tip is at least H - 8 (Core only keeps a best chain lock it verified), so the signing quorum is selectable and verifychainlock accepts it. A Core more than 8 blocks behind answers false instead of throwing "No quorum found": rejected before and after. So genuine chain locks are rejected in no new case, and forged ones are now rejected by every honest validator instead of depending on each validator's Core. A committed protocol 14 block with a genuine chain lock, which every honest proposer proposes, replays the same way. A block whose forged chain lock v0 accepted through the early return (a Byzantine proposer with rc.1..rc.3 validators holding 2/3) can never be applied by this version; only pre-release protocol 14 networks can hold one. verifychainlock gets the block hash in display order: Drive builds the argument with BlockHash's Display, so the internal-order bug of the pinned rust-dashcore submit_chain_lock (rust-dashcore #1148) does not apply. A test against the mock JSON-RPC Core pins the wire arguments with mainnet chain lock 2555653. Protocol 14 only, in place: v1 is not in any release (5.0.0-rc.1..rc.3 run v0 at protocol 14), testnet and mainnet run protocol 13. No version table changes; item 96 of v14.rs and the V10 table docs describe the Core check. Mixed rc.3/rc.4 validators now also disagree on a forged chain lock in v0's deferred range at a height the validator's Core already has a chain lock for: rc.3 accepts it through the early return, rc.4 rejects it. Only a Byzantine proposer can produce one. Drive now calls verifychainlock on the consensus path: dashmate's drive_consensus RPC whitelist has always allowed it; a deployment without dashmate must allow it too. The strategy tests' mock Core accepts verifychainlock as it accepts submitchainlock; run_chain_lock_update_quorums_not_changing defers a chain lock and needs it. Addresses thepastaclaw's review on #5454. Refs #5438. Test would have caught this: verify_chain_lock with a forged chain lock at 1122 (newly deferred by v1) and 1136 (past the DKG interval end, deferred by v0 too), mock Core best chain lock 1140: ✖ before fix (accepted, Core synced), ✔ after (rejected, submitchainlock not asked). The byte order test fails when the wrapper sends hex::encode(block_hash). The v0/v1 differential test now also runs v1 through the dispatcher against a Core that fails every chain lock: exactly the chain locks the held quorums leave to Core are asked about and rejected. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…rifychainlock (PV14) A chain lock that verify_chain_lock_locally v1 leaves to Core (None) went straight to Core's submitchainlock. Dash Core returns its best chain lock height from submitchainlock without checking the signature when that height is at or above the submitted one (rpc/quorums.cpp, v23.1.8), and Drive turns that answer into "Core is synced". So a Byzantine proposer's forged chain lock at an already chain-locked height was accepted by every validator whose Core had a chain lock at or above it. verify_chain_lock_locally v1 now asks Core's verifychainlock whenever the check against the quorums Platform holds cannot decide, on every such path: - false: Some(false), the proposal is rejected and submitchainlock is not called; - true: None, unchanged: submitchainlock still decides whether Core is synced; - RPC error: Err(CoreRpc), which run_block_proposal turns into InvalidChainLock, as it already does for a submitchainlock error there. The check against the held quorums moves unchanged into verify_chain_lock_with_held_quorums, which the local-logic tests call. A false from Core is logged as a warning naming both causes (an invalid signature, or Core missing the block at H - 8); the proposal error is the existing local-rejection one. Both RPCs run the same chainlock::VerifyChainLock. For a genuine chain lock above Core's best they give the same verdict; at or below it, Core's tip is at least H - 8 (Core only keeps a best chain lock it verified), so the signing quorum is selectable and verifychainlock accepts it. A Core more than 8 blocks behind answers false instead of throwing "No quorum found": rejected before and after. So genuine chain locks are rejected in no new case, and forged ones are now rejected by every honest validator instead of depending on each validator's Core. A committed protocol 14 block with a genuine chain lock, which every honest proposer proposes, replays the same way. A block whose forged chain lock v0 accepted through the early return (a Byzantine proposer with rc.1..rc.3 validators holding 2/3) can never be applied by this version; only pre-release protocol 14 networks can hold one. verifychainlock gets the block hash in display order (BlockHash's Display), as Core parses it; this branch's rust-dashcore pin also sends display order to submitchainlock (rust-dashcore #1148). The wire-level byte order test is on v5.0-dev, whose pin still has the submitchainlock bug; its mock JSON-RPC Core is not on this branch. Protocol 14 only, in place: v1 is not in any release (5.0.0-rc.1..rc.3 run v0 at protocol 14), testnet and mainnet run protocol 13. No version table changes; item 96 of v14.rs and the V10 table docs describe the Core check. Mixed rc.3/rc.4 validators now also disagree on a forged chain lock in v0's deferred range at a height the validator's Core already has a chain lock for: rc.3 accepts it through the early return, rc.4 rejects it. Only a Byzantine proposer can produce one. Drive now calls verifychainlock on the consensus path: dashmate's drive_consensus RPC whitelist has always allowed it; a deployment without dashmate must allow it too. The strategy tests' mock Core accepts verifychainlock as it accepts submitchainlock; run_chain_lock_update_quorums_not_changing defers a chain lock and needs it. Same change as on v5.0-dev (#5459), so the forward merge sees identical edits apart from the BLS API lines. Addresses thepastaclaw's review on #5454. Refs #5438. Test would have caught this: verify_chain_lock with a forged chain lock at 1122 (newly deferred by v1) and 1136 (past the DKG interval end, deferred by v0 too), mock Core best chain lock 1140: ✖ before fix (accepted, Core synced), ✔ after (rejected, submitchainlock not asked). The v0/v1 differential test now also runs v1 through the dispatcher against a Core that fails every chain lock: exactly the chain locks the held quorums leave to Core are asked about and rejected. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
At head 7f867eb, deferred ChainLocks are authenticated before synchronization, fixing the prior security finding. One blocking protocol-14 compatibility issue and one non-blocking parameter-ownership suggestion remain. This was a static review only; the supplied exact-head CI snapshot had runner selection queued and no completed build/test result.
🔴 1 blocking | 🟡 1 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 6: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 7: gemini-3.8-flash-high (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — verify_chain_lock_locally/v1/mod.rs introduces intricate quorum-availability and signing-height logic that changes consensus-facing ChainLock signature rejection decisions, while the version tables activate that behavior at protocol 14. - Phase 1 reviewers:
gemini-3.8-flash-high— general (completed, effort high); agentphase1-reviewer,gemini-3.8-flash-high— rust-quality (completed, effort high); agentphase1-reviewer - Phase 1 model:
gemini-3.8-flash-high— antigravity quota: weekly 98% left, 5h 92% left - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v10.rs`:
- [BLOCKING] packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_method_versions/v10.rs:94: Preserve protocol 14's existing ChainLock verifier selection
Changing this slot changes the acceptance rules for protocol 14, rather than activating the correction at a new protocol boundary. The incompatibility is concrete: the dispatcher regression demonstrates valid certificates rejected by the previous selection but accepted by this one, while the forged-certificate regressions demonstrate the reverse. The new v14 documentation also explicitly acknowledges that a previously committed block containing a forged deferred certificate cannot be replayed by v1. I checked #5459's coordinated-upgrade instructions and its history-audit/state-sync/reset policy; those explain how operators can manage the incompatibility, but they do not establish the consensus-inertness required by the repository's frozen-snapshot rule. Prerelease status does not make an already accepted protocol-14 history replayable. Preserve V10's selection at 0 and select v1 through a new method-table snapshot referenced by PLATFORM_V15, with dispatcher coverage preserving protocol 14's verdicts. Shipping an intentional protocol-14 history break instead requires an explicit exception to that compatibility invariant, not an argument that genuine certificates alone preserve replay.
In `packages/rs-drive-abci/src/execution/platform_events/core_chain_lock/verify_chain_lock_locally/v1/mod.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/execution/platform_events/core_chain_lock/verify_chain_lock_locally/v1/mod.rs:307-309: Keep DKG timing parameters in the protocol version tables
(existing thread: https://github.com/dashpay/platform/pull/5454#discussion_r4283288224)
These interval and inclusive mining-window values determine whether Platform rejects a certificate against its held quorums or asks Core, so they are changeable protocol parameters. The book's parameter-ownership convention puts such values in PlatformVersion constants rather than an implementation generation. Freezing the corrected Core schedule is appropriate and avoids the discrepant dependency-owned table, but a frozen PlatformVersion-owned schedule provides the same protection against dependency drift. Store the schedule alongside DriveAbciCoreChainLockMethodVersionsAndConstants and read it through platform_version; keep the QuorumType mapping in drive-abci so platform-version needs no DPP or dashcore dependency. This allows a future schedule-only adjustment through a data snapshot without duplicating verification logic or editing a frozen implementation.
|
Waiting for bot review — coderabbitai not yet · thepastaclaw requested changes — dismiss the review or push a fix, 1 thread unresolved — resolve it. Wait for the missing reviews, or a writer can post |
|
Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix; thepastaclaw left review threads unresolved; resolve them. |
…s a forged chain lock
The forged-chain-lock test asserted `core_is_synced == None` but its
`submit_chain_lock` expectation allowed any number of calls, so an
implementation that submitted the chain lock before checking its signature,
then returned the rejection, still passed. `.times(0)` makes the mock enforce
verify-before-submit, like the neighbouring rejection and RPC-error tests.
Checked with a mutant that calls `submit_chain_lock` before
`verify_chain_lock` in `verify_chain_lock_locally_v1`:
✔ old test passes on the mutant (blind), ✖ new test fails it
("submit_chain_lock: Expectation(<anything>) should not have been called"),
✔ new test passes on the real code. All 48 chain_lock lib tests pass.
Addresses review suggestion on #5459.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…only Protocol 14 keeps verify_chain_lock_locally v0. Testnet activated protocol 14 on 5.0.0-rc.3, and v1 rejects ChainLocks v0 accepts: a forged ChainLock that v0 leaves to Core passes through submitchainlock's early return when Core already has a ChainLock at or above its height, while v1 has verifychainlock check its signature first. A protocol 14 block committed with such a ChainLock would not replay under v1, so v1 moves to the next protocol boundary instead of changing protocol 14 in place. - DRIVE_ABCI_METHOD_VERSIONS_V10 is back to the base table (verify_chain_lock_locally 0, as on v5.0-dev), and v14.rs loses the entry this branch added. - DRIVE_ABCI_METHOD_VERSIONS_V11 is V10 with verify_chain_lock_locally 1, in struct update form, used only by PLATFORM_V15; v15.rs lists the change. #4648 and #4652 also add a V11 for protocol 15; whichever lands second adds its slot to the existing table. - The dispatcher gate test keeps protocols 13 and 14 on v0's rejection of a ChainLock signed by a newly mined quorum and moves the deferral to 15. - A new dispatcher test keeps protocols 13 and 14 leaving a forged ChainLock past the DKG interval to Core without a verifychainlock call, and rejects it at 15 once verifychainlock fails. - A new verify_chain_lock test keeps protocol 14 accepting that ChainLock through submitchainlock's early return, which replaying protocol 14 blocks needs. - The v0-against-v1 differential test also checks that protocol 14 gives v0's verdict on every history without asking Core; v0's own test runs at protocol 14. Test would have caught a wrong selection: with V10's slot at 1 (protocol 14 on v1) 4 chain lock tests fail, with V11's slot at 0 (protocol 15 on v0) 6 fail (✖); with the tables as committed all 49 pass (✔). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Waiting for bot review — coderabbitai not yet · thepastaclaw ✓. Wait for the missing reviews, or a writer can post |
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
At bd5fe9b, the verifier change is correctly isolated to protocol 15, historical production behavior is preserved, and deferred ChainLocks are authenticated before submission. Three prior findings are fixed; two nonblocking suggestions remain concerning timing-parameter ownership and Core-fallback organization. Validation was static only: current CI reports Rust workspace tests and the Drive image build pending, with no failed code checks reported.
🟡 2 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 6: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: tenderdash-consensus-security); reviewer 8: gemini-3.8-flash-high (agent: phase1-reviewer, role: rust-quality); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 13: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 14: gpt-6.1-sol (agent: phase2-reviewer, role: tenderdash-consensus-security); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The new verify_chain_lock_locally_v1 implementation intricately changes consensus-path ChainLock signature acceptance using quorum mining schedules, historical quorum coverage, and fallback verification through Core RPC. - Phase 1 reviewers:
gemini-3.8-flash-high— general (completed, effort high); agentphase1-reviewer,gemini-3.8-flash-high— rust-quality (completed, effort high); agentphase1-reviewer - Phase 1 model:
gemini-3.8-flash-high— antigravity quota: weekly 71% left, 5h 100% left - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— tenderdash-consensus-security (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— tenderdash-consensus-security (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-drive-abci/src/execution/platform_events/core_chain_lock/verify_chain_lock_locally/v1/mod.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/execution/platform_events/core_chain_lock/verify_chain_lock_locally/v1/mod.rs:53-64: Centralize fallback authentication in the Core verifier
The signature check protects the production submission path, but placing it inside the local verifier creates two concrete inconsistencies. For a deferred certificate, verify_chain_lock(..., false, ...) now calls verifychainlock here and again in verify_chain_lock_through_core_v0; a negative Core verdict instead becomes found_valid_locally: Some(false), whose documented meaning is verification against platform state. Consequently, proposal validation reports that Core rejection as an invalidity established from platform state. Move fallback authentication into a protocol-15-selected verify_chain_lock_through_core v1, leaving the local generation to return the held-quorum verdict. That keeps authentication and synchronization together, avoids duplicate verification in the non-submitting route, and attributes Core verdicts correctly. The current production caller is authenticated, so this is an organization and diagnostic suggestion, not a remaining acceptance vulnerability.
- [SUGGESTION] packages/rs-drive-abci/src/execution/platform_events/core_chain_lock/verify_chain_lock_locally/v1/mod.rs:307-309: Keep DKG timing parameters in the protocol version tables
(existing thread: https://github.com/dashpay/platform/pull/5454#discussion_r4283288224)
These interval and inclusive mining-window triples control whether a failed local signature check rejects the certificate or delegates to Core. They match Dash Core v23.1.8, and freezing Platform-owned values instead of consulting the pinned rust-dashcore table is justified. However, book/src/versioning/platform-version.md explicitly places constants that could change in a future protocol in version tables; its implementation-local exception covers immutable encoding/storage constants, not timing schedules. Keeping the schedule inside v1 means a future timing-only adjustment requires another implementation generation or an edit to shipped code. Put the same frozen schedule in a dependency-free representation under rs-platform-version, select it through the chain-lock constants snapshot, and read it through platform_version. Preserve historical selections and the conservative fallback for unsupported types. This retains dependency-bump isolation without changing the current verdicts.
|
@coderabbitai review No review for |
…-chainlock-verify
…_lock_through_core v1 (PV15) Addresses two review suggestions on the protocol 15 chain lock verification. Core's verifychainlock check of a chain lock Platform leaves to Core moves from verify_chain_lock_locally v1 into a new verify_chain_lock_through_core v1, selected by DRIVE_ABCI_METHOD_VERSIONS_V11 (protocol 15 only): - verify_chain_lock_locally v1 is now only the check against the quorums Platform holds and never calls Core. - verify_chain_lock_through_core v1 asks verifychainlock first. A failure is returned as Core's verdict, so the block proposal reports it as invalid based on a core request instead of on platform state, and the chain lock is not submitted. An accepted one is submitted through make_sure_core_is_synced_to_chain_lock for the sync status, as before. - Accept/reject verdicts and the RPC sequence of the block proposal route are unchanged; the non-submitting route asks verifychainlock once instead of twice. Dash Core's DKG mining schedules, which decide whether a failed check rejects a chain lock or leaves it to Core, move from a match in verify_chain_lock_locally v1 into the version tables: DriveAbciCoreChainLockMethodVersionsAndConstants.dkg_mining_schedules, Some(schedules) in V11 and None in V1 to V10 and the mock table, which select verify_chain_lock_locally 0 and never read it. v1 reads it through platform_version, fails with CorruptedCodeExecution on None, and uses checked_rem so a zero interval cannot panic. Protocol 14 and earlier keep verify_chain_lock_locally 0 and verify_chain_lock_through_core 0; the only change to their tables is the unread None field. Tests: - verify_chain_lock: a forged deferred chain lock Core fails is rejected with found_valid_by_core Some(false), not found_valid_locally Some(false) (✖ before, ✔ after); the non-submitting route asks verifychainlock once (✖ before: twice, ✔ after); over every protocol version, a forged chain lock at a height v1 newly defers is never accepted, which also guards pairing verify_chain_lock_locally 1 with verify_chain_lock_through_core 1. - verify_chain_lock_through_core dispatcher: protocols 13 and 14 submit without verifychainlock; 15 and latest do not submit after it fails (✖ before, ✔ after). - verify_chain_lock_locally: the local check asks Core nothing at any protocol version; the v0/v1 differential test also runs the outer verification on both routes against a Core that fails every signature. - v1 reads the schedules from platform_version (an empty table leaves a chain lock to Core that the latest table rejects, None errors). This one does not compile against the previous code, which has no such field, so it has no behavioural red. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…s and Core sync answers
Review follow-ups to moving the Core signature check of deferred chain locks
into verify_chain_lock_through_core v1:
- The forged-chain-lock guard over every protocol version now goes through
the verify_chain_lock dispatcher. Mutation check: with V11's
verify_chain_lock_through_core set back to 0 it fails ("protocol version
15 accepted a forged chain lock"), 7 tests fail in total (✖); with the
table as committed all pass (✔).
- A chain lock left to Core that verifychainlock accepts while Core's best
chain lock is behind it is not accepted (core_is_synced Some(false)).
- The v0/v1 differential test also runs verify_chain_lock at protocol 14 on
both routes for every history and chain lock, pinning its replay
verdicts: no verifychainlock when submitting, and submitchainlock for
every chain lock v0 does not reject.
- A DKG mining schedule with an interval of 0 errors instead of dividing by
it, and every platform version's schedules are unique per quorum type
with the mining window inside the interval.
- Doc fixes: the error sources of verify_chain_lock_through_core and
verify_chain_lock_locally, and v15.rs naming submitchainlock's early
return.
No behaviour change; tests and comments only.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Waiting for bot review — coderabbitai not yet · thepastaclaw ✓. Wait for the missing reviews, or a writer can post |
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
At head 957da3e, the PV15-only activation, version-owned DKG schedules, and Core authentication before submission address all five prior findings while preserving historical verifier behavior. One nonblocking allocation improvement remains in the new local verifier; no blocking defects were confirmed. This was a static review: current CI reports Rust workspace tests and the previously pending JavaScript suites passing, while PR Hygiene remains pending.
🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: tenderdash-consensus-security); reviewer 7: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 8: gemini-3.8-flash-high (agent: phase1-reviewer, role: rust-quality); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 13: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 14: gpt-6.1-sol (agent: phase2-reviewer, role: tenderdash-consensus-security); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The intricate changes to verify_chain_lock_locally/v1/mod.rs and verify_chain_lock_through_core/v1/mod.rs alter consensus-critical ChainLock signature acceptance and deferral rules, with protocol-version dispatch and quorum mining-window calculations. - Phase 1 reviewers:
gemini-3.8-flash-high— general (completed, effort high); agentphase1-reviewer,gemini-3.8-flash-high— rust-quality (completed, effort high); agentphase1-reviewer - Phase 1 model:
gemini-3.8-flash-high— antigravity quota: weekly 71% left, 5h 100% left - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— tenderdash-consensus-security (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— tenderdash-consensus-security (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-drive-abci/src/execution/platform_events/core_chain_lock/verify_chain_lock_locally/v1/mod.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/execution/platform_events/core_chain_lock/verify_chain_lock_locally/v1/mod.rs:151-152: Avoid heap allocation when reversing quorum hash byte array
`quorum_hash.to_byte_array()` already returns an owned `[u8; 32]`, so converting it to a `Vec` introduces an unnecessary allocation during local ChainLock verification. Reverse the array directly and retain the existing slice input to the hash engine; this preserves the digest bytes. Apply the same change to the second-quorum branch at lines 197–198 in this new generation, as the test helper already does.
…n_lock_locally v1 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Waiting for bot review — coderabbitai not yet · thepastaclaw ✓. Wait for the missing reviews, or a writer can post |
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Static verification of the complete diff at 2cd9d44 found no remaining actionable issues, and all six prior findings are fixed. Protocol 15 activates the paired local and Core verification generations, while historical production implementations and protocol 1–14 selections remain unchanged. No builds or tests were run locally; current CI shows Rust workspace tests, JS builds, and Kotlin checks pending, with no failed code checks reported.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 6: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: tenderdash-consensus-security); reviewer 8: gemini-3.8-flash-high (agent: phase1-reviewer, role: rust-quality); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 13: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 14: gpt-6.1-sol (agent: phase2-reviewer, role: tenderdash-consensus-security); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The intricate changes in verify_chain_lock_locally/v1/mod.rs and verify_chain_lock_through_core/v1/mod.rs alter consensus-critical ChainLock acceptance and signature verification, including quorum coverage decisions and protocol-15 activation. - Phase 1 reviewers:
gemini-3.8-flash-high— general (completed, effort high); agentphase1-reviewer,gemini-3.8-flash-high— rust-quality (completed, effort high); agentphase1-reviewer - Phase 1 model:
gemini-3.8-flash-high— antigravity quota: weekly 71% left, 5h 100% left - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— tenderdash-consensus-security (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— tenderdash-consensus-security (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
|
Bots are done — your move: post |
Issue being fixed or feature implemented
Fixes #5438.
Drive rejects valid ChainLocks in block proposals ("received a chain lock ... that we figured out was invalid based on platform state"), failing rounds until Core produces another ChainLock.
A ChainLock for core height
His signed by a quorum Core picks from the quorums active atH - 8. Drive checks it against the two chain lock quorum sets it stores: the set active at its last committed core heightC, and the set before it.verify_chain_lock_locallyv0 treats a failed check as a definitive rejection in two cases where Drive does not actually hold the signing quorums:Cin the same DKG interval. v0 only hands a ChainLock to Core whenH - 8is past the end ofC's DKG interval, assuming the quorums cannot change before then. Quorums are mined in the middle of the interval (llmq_test and llmq_50_60 at offsets 10–18 of 24, llmq_400_60 at 20–28 of 288). Once Platform's core height lags Core by more than 8 blocks, a ChainLock signed by the new quorum is rejected locally without asking Core.Seen in the 4.1.1 → 5.0.0-rc.2 local upgrade rehearsal (7 evonodes, llmq_test). Right after protocol v14 activated, Platform block 1122 took 4 rounds (3 min 13 s):
Each such ChainLock is wrongly rejected with probability 2 / (active quorums + 1): 2/3 on local networks, 2/5 on mainnet and devnets, 2/25 on testnet. The rejection is a deterministic function of the platform state, so all validators agree. It costs liveness, not safety, and every proposer re-proposes Core's best ChainLock until Core has a new one. The logic is unchanged since at least v4.1.1.
Because Drive submits every ChainLock it does not reject to Core anyway (
make_sure_core_is_synced), the local check only ever matters as a rejection. The fix is therefore to reject locally only when Drive provably holds the quorums active at the signing height, and to leave the rest to Core.What was done?
verify_chain_lock_locallyv1, a copy of v0 with three decision changes:After a failed check against the current quorums, return
None(Core decides) when a quorum commitment of the chain lock type can have been mined betweenCand the signing height. Core only mines commitments inside each DKG interval's mining window. The first such height comes fromcore_chain_lock.dkg_mining_schedulesin the version tables, which holds Dash Core'sllmq/params.hvalues for the non-rotating types, keyed by Core's LLMQ type number. Rotating and unknown types fall back to "any height afterC".The schedules are frozen in the protocol 15 tables rather than read from rust-dashcore's
LLMQType::params(), for two reasons. rust-dashcore's table disagrees with Core for llmq_test_platform and llmq_devnet_platform (interval 288 instead of 24) and for llmq_devnet_dip0024 (mining offsets). llmq_devnet_platform is Drive's default devnet chain lock type. Freezing them in the tables also means a dependency bump cannot change protocol 15's behaviour, and a later protocol can change a schedule with a table edit instead of a new generation. v1 reads them throughplatform_version; a table without them (None), or a schedule with an interval of 0, is aCorruptedCodeExecutionerror rather than a panic.When both stored sets fail, return
Noneinstead of rejecting.Drop v0's
round >= 10branch, which only logged.v1 differs from v0 only by turning some
Some(false)intoNone. EverySome(true)andNoneis unchanged, and every remaining local rejection is on a quorum set Drive holds exactly. WhenCis already past its interval's mining window (most positions on mainnet), v1 keeps v0's local rejection up to the interval end. v1 never calls Core.verify_chain_lock_through_corev1 has Core'sverifychainlockcheck the signature of every ChainLock Drive leaves to Core, beforesubmitchainlocksees it. Dash Core'ssubmitchainlockreturns its best ChainLock height without checking the signature when that height is at or above the submitted one (v23.1.8rpc/quorums.cpp), and Drive reads that answer as "Core is synced", so a forged ChainLock at an already chain-locked height passed.false: Core's verdict,found_valid_by_core: Some(false), so the proposal is rejected as "invalid based on a core request" (not as invalid based on platform state), andsubmitchainlockis not called. A warning names Core's verdict.true: submitted throughmake_sure_core_is_synced_to_chain_lock, which decides whether Core is synced exactly as v0's submit branch does.verifychainlockis the whole answer and is asked once.InvalidChainLock), as asubmitchainlockerror already is on that path.chainlock::VerifyChainLock, and at or below it Core's tip is at leastH - 8(Core keeps only a best ChainLock it verified), so the signing quorum is selectable. Forged ChainLocks are now rejected by every honest validator instead of depending on each one's Core.verify_chain_lock_locally1 leaves to Core ChainLocks that 0 rejected, so it is only safe together withverify_chain_lock_through_core1. A test over every protocol version guards that pairing.Version selection.
DRIVE_ABCI_METHOD_VERSIONS_V11isDRIVE_ABCI_METHOD_VERSIONS_V10withverify_chain_lock_locally: 1,verify_chain_lock_through_core: 1anddkg_mining_schedules: Some(..)incore_chain_lock, written in struct update form, and onlyPLATFORM_V15uses it;v15.rslists the change.PLATFORM_V14andv14.rsare unchanged, andDRIVE_ABCI_METHOD_VERSIONS_V10and earlier only gain the unreaddkg_mining_schedules: None(see below), so protocol version 14 and earlier keepverify_chain_lock_locally0 andverify_chain_lock_through_core0.Other open v5.1 PRs also add a
DRIVE_ABCI_METHOD_VERSIONS_V11for protocol 15: feat(drive-abci)!: state sync via ABCI snapshots with reduced platform state (protocol v15) #4648 (state sync) and feat(drive)!: preserve composite document history across protocol activation #4652 (document history). Whichever lands second rebases and adds its slot to the existing V11 instead of creating another table.test::helpers::chain_lock_quorums: test quorums with known keys and ChainLocks signed the way Core signs them.The strategy tests' mock Core now also answers
verifychainlock(accepting every ChainLock, as itssubmitchainlockalready does).In-place changes to shipped generations
DRIVE_ABCI_METHOD_VERSIONS_V1toV10(protocol versions 1 to 14) and the mock tablemocks/v3_test.rsgain the new struct fieldcore_chain_lock.dkg_mining_schedules: None. Consensus cannot change there: those tables selectverify_chain_lock_locally0, which never reads the field, and no other code reads it. No shipped implementation module is edited (verify_chain_lock_locallyv0,verify_chain_lock_through_corev0,verify_chain_lockv0 andmake_sure_core_is_synced_to_chain_lockv0 are byte-identical outside their test modules; only the test modules ofverify_chain_lockv0 andverify_chain_lock_locallyv0 changed).How Has This Been Tested?
cargo test -p drive-abci --lib: 4321 passed, 0 failed, 17 ignored (existing#[ignore]s); the chain lock tests (cargo test -p drive-abci --lib chain_lock): 54 passed.cargo test -p platform-version: 27 passed, 0 failed.RUST_MIN_STACK=4194304 cargo test -p drive-abci --test strategy_tests: 115 passed, 0 failed, 4 ignored.cargo clippy -p drive-abci -p platform-version --all-features --all-targets -- -D warnings,cargo check --workspace --all-targetsand rustfmt: clean.Version gate, through the dispatcher:
C(case A) is rejected at protocols 13 and 14 and left to Core at 15 and the latest.verify_chain_lock_through_core: a forged ChainLock is submitted without averifychainlockcall at protocols 13 and 14 (times(0)), and at 15 and the latest is not submitted afterverifychainlockfails.verify_chain_lockat any protocol version inPLATFORM_VERSIONS. Pairing V11'sverify_chain_lock_locally1 withverify_chain_lock_through_core0 fails it ("protocol version 15 accepted a forged chain lock").verify_chain_lock, protocol 14 still accepts that forged ChainLock throughsubmitchainlock's early return (Core's best ChainLock at 1140), without callingverifychainlock, which replaying protocol 14 blocks needs. The same ChainLock is rejected under the latest protocol version.verify_chain_lockat protocol 14 on both routes: noverifychainlockwhen submitting, andsubmitchainlockfor every ChainLock v0 does not reject, as the committed protocol 14 blocks were verified.verify_chain_lock_locally1 at protocol 14 (V10's slot set to 1) fails 3 tests; keeping 0 at protocol 15 (V11's slot set to 0) fails 7 tests; V11 withverify_chain_lock_through_core0 fails 7 tests (✖); all pass with the tables as committed (✔).v1:
Some(false)under the latest protocol version (✖ before, ✔ after).Some(true)and a wrong signerSome(false).Some(true)andNoneunchanged, and neither asks Core;verify_chain_lockon both routes, against a Core that fails every signature and already has a ChainLock above all of them:verifychainlockis asked exactly about the ChainLocks v1 leaves to Core (one call each), which are rejected as Core's verdict;submitchainlockis called only for ChainLocks v1 verified.verify_chain_lockunder the latest protocol version:verifychainlockasked once for that ChainLock, thensubmitchainlock;times(0)for both RPCs);found_valid_by_core: Some(false),found_valid_locally: None), andsubmitchainlockis never called (times(0)) (✖ before theverifychainlockstep: accepted through the early return; ✖ when the check sat inverify_chain_lock_locally: reported as found invalid locally; ✔ after);verifychainlockis asked once about a ChainLock left to Core, and its verdict is the result (✖ when the check sat inverify_chain_lock_locally: asked twice; ✔ after);verifychainlockaccepts while Core's best ChainLock is 10 blocks behind it is not accepted (core_is_synced: Some(false));verifychainlockRPC error reaches the proposal asErr(CoreRpc), withoutsubmitchainlock.first_possible_quorum_change_afteris checked at the window boundaries of all eleven non-rotating types, including llmq_devnet_platform, against the latest protocol version's table. The expected values are keyed by dashcore'sQuorumTypeand the table by Core's LLMQ type number, so this also checks the numbering. Rotating types (including llmq_devnet_dip0024) and unknown types fall back.platform_version: with an empty table, a wrong signer the latest table rejects locally is left to Core;Noneand a zero interval areCorruptedCodeExecutionerrors. Every platform version's schedules are unique per quorum type with the mining window inside the interval.Breaking Changes
Consensus change at protocol version 15. A protocol 15 block proposal whose ChainLock was signed by quorums Drive does not hold is checked by Core's
verifychainlockinstead of being rejected, and a forged ChainLock Drive leaves to Core is rejected even when Core already has a ChainLock at or above its height. Protocol version 14 and earlier behave as before.Checklist:
structure.rs, regeneratedgrovedb-structure.json, and checked the structure viewer link posted on this pull requestFor repository code-owners and collaborators only
🤖 Generated with Claude Code
PR Hygiene ·
2cd9d44/self-reviewedWhen every merge requirement is met, the
PR Hygienecheck passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.